-
Notifications
You must be signed in to change notification settings - Fork 13
[INTERNAL] Refactoring #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, the refactoring looks good to me.
Not sure if you would like to modernize the code in general. Some methods still use that = this (instead of arrow functions) and could use async (e.g. cacheTheme).
Sorry, I missed to explain that: |
Using new language features is a good thing but I didn't focus on changing all the code. Can be done as another follow-up, along with fixing the |
- Convert Promises to async/await - Use template literals
Reducing the monolithic index.js script to improve testability of individual functionalities rather then just testing the whole build based on input/output.
Might be different in older Node.js versions
Rebased on top of master to also test on Node 16 (see #183) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Fixing code styles issues later is totally acceptable, same for modernising.
Thanks 👍🏻 |
Note: I'm using rebase and merge to keep the history and have separate commits for changing the code and moving it to other files. |
JIRA: CPOUI5FOUNDATION-355